feat: Add webhook update method with isEnabled/statusChangeReason by jacalata · Pull Request #1806 · tableau/server-client-python

jacalata · 2026-06-26T23:20:17Z

Summary

Adds Webhooks.update(webhook_item) (REST API v3.6) for modifying an existing webhook's name, event, destination URL, and enabled state
Adds is_enabled (bool) and status_change_reason (str) fields to WebhookItem, parsed from the isEnabled and statusChangeReason XML attributes returned by the server
RequestFactory.Webhook.update_req() serializes all updatable fields; is_enabled is omitted when None to support partial updates; statusChangeReason is intentionally not serialized (server-set, read-only)
Raises MissingRequiredFieldError if the webhook item has no ID
Handles both webhook-source-event-* (legacy) and webhook-event-* (newer) event name prefixes in the event setter

Schema compliance

isEnabled and statusChangeReason are both defined on webhookType in ts-api_3_29.xsd. The update_req() child element structure (webhook-source, webhook-destination) matches the schema. webhook-event-* style event names are a live API extension not yet reflected in the published XSD; handling them is consistent with the pre-existing behavior in create_req().

Test plan

python -m pytest test/test_webhook.py -v -- 22 tests, all pass
mypy: no issues
Verify server.webhooks.update(item) returns updated WebhookItem with correct fields
Verify updating only is_enabled=False leaves name/url/event unchanged on server
Verify MissingRequiredFieldError raised when item has no ID
Verify isEnabled attribute absent from request XML when is_enabled is None

🤖 Generated with Claude Code

Fixes #1135 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Tighten test_update_missing_id to assert MissingRequiredFieldError specifically. Add tests for update_req serializing url and event, omitting isEnabled when None, partial (name-only) updates, and correct parsing of isEnabled="false" from XML. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

The return-type annotation added in the branch accidentally dropped the docstring that was on test_event_setter_none. Restore it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

github-actions · 2026-06-26T23:21:01Z

Coverage Report

File	Stmts	Miss	Cover	Missing
tableauserverclient
__init__.py	5	0	100%
config.py	15	0	100%
datetime_helpers.py	25	1	1	96%
exponential_backoff.py	20	0	100%
filesys_helpers.py	31	0	100%
namespace.py	26	3	3	88%
tableauserverclient/bin
__init__.py	2	0	100%
_version.py	358	212	212	41%
tableauserverclient/helpers
__init__.py	1	0	100%
logging.py	2	0	100%
strings.py	31	1	1	97%
tableauserverclient/models
__init__.py	46	0	100%
collection_item.py	41	7	7	83%
column_item.py	55	32	32	42%
connection_credentials.py	35	11	11	69%
connection_item.py	94	14	14	85%
custom_view_item.py	144	21	21	85%
data_acceleration_report_item.py	54	1	1	98%
data_alert_item.py	158	4	4	97%
data_freshness_policy_item.py	155	15	15	90%
database_item.py	207	36	36	83%
datasource_item.py	300	12	12	96%
dqw_item.py	104	5	5	95%
exceptions.py	4	0	100%
extensions_item.py	132	4	4	97%
extract_item.py	44	4	4	91%
favorites_item.py	69	8	8	88%
fileupload_item.py	19	0	100%
flow_item.py	149	10	10	93%
flow_run_item.py	71	0	100%
group_item.py	89	6	6	93%
groupset_item.py	49	7	7	86%
interval_item.py	182	32	32	82%
job_item.py	192	10	10	95%
linked_tasks_item.py	79	1	1	99%
location_item.py	29	2	2	93%
metric_item.py	129	13	13	90%
oidc_item.py	63	3	3	95%
pagination_item.py	34	1	1	97%
permissions_item.py	111	12	12	89%
project_item.py	207	31	31	85%
property_decorators.py	100	18	18	82%
reference_item.py	26	2	2	92%
revision_item.py	59	1	1	98%
schedule_item.py	209	6	6	97%
server_info_item.py	37	7	7	81%
site_item.py	636	13	13	98%
subscription_item.py	101	2	2	98%
table_item.py	119	18	18	85%
tableau_auth.py	61	25	25	59%
tableau_types.py	27	1	1	96%
tag_item.py	15	0	100%
target.py	6	0	100%
task_item.py	56	2	2	96%
user_item.py	310	18	18	94%
view_item.py	220	16	16	93%
virtual_connection_item.py	64	8	8	88%
webhook_item.py	88	1	1	99%
workbook_item.py	362	16	16	96%
tableauserverclient/server
__init__.py	9	0	100%
exceptions.py	4	0	100%
filter.py	29	1	1	97%
pager.py	33	1	1	97%
query.py	143	15	15	90%
request_factory.py	1351	195	195	86%
request_options.py	386	5	5	99%
server.py	188	23	23	88%
sort.py	6	0	100%
tableauserverclient/server/endpoint
__init__.py	35	0	100%
auth_endpoint.py	77	11	11	86%
custom_views_endpoint.py	152	12	12	92%
data_acceleration_report_endpoint.py	21	0	100%
data_alert_endpoint.py	94	23	23	76%
databases_endpoint.py	111	30	30	73%
datasources_endpoint.py	323	33	33	90%
default_permissions_endpoint.py	44	3	3	93%
dqw_endpoint.py	45	16	16	64%
endpoint.py	212	20	20	91%
exceptions.py	77	6	6	92%
extensions_endpoint.py	31	0	100%
favorites_endpoint.py	94	22	22	77%
fileuploads_endpoint.py	51	0	100%
flow_runs_endpoint.py	62	9	9	85%
flow_task_endpoint.py	21	2	2	90%
flows_endpoint.py	198	53	53	73%
groups_endpoint.py	126	9	9	93%
groupsets_endpoint.py	72	7	7	90%
jobs_endpoint.py	67	9	9	87%
linked_tasks_endpoint.py	37	0	100%
metadata_endpoint.py	88	14	14	84%
metrics_endpoint.py	55	6	6	89%
oidc_endpoint.py	42	1	1	98%
permissions_endpoint.py	44	3	3	93%
projects_endpoint.py	178	24	24	87%
resource_tagger.py	127	35	35	72%
schedules_endpoint.py	119	11	11	91%
server_info_endpoint.py	36	10	10	72%
sites_endpoint.py	130	27	27	79%
subscriptions_endpoint.py	56	14	14	75%
tables_endpoint.py	110	36	36	67%
tasks_endpoint.py	63	6	6	90%
users_endpoint.py	183	8	8	96%
views_endpoint.py	150	9	9	94%
virtual_connections_endpoint.py	113	10	10	91%
webhooks_endpoint.py	67	4	4	94%
workbooks_endpoint.py	338	22	22	93%
TOTAL	12055	1418	88%

bcantoni · 2026-07-23T04:40:33Z

Code Review

Clean, well-structured PR. Here are the issues worth addressing before merge:

Issues

Convention deviation — update uses from_response()[0] instead of copy.copy + _parse_common_tags

webhooks_endpoint.py:150 — The project's documented update pattern (used in users_endpoint.py:208, datasources_endpoint.py:353) is:

updated = copy.copy(item)
return updated._parse_common_tags(server_response.content, ns)

This preserves locally-set fields that the server's partial response omits. Using from_response()[0] works here because the server always returns a complete webhook and WebhookItem has no _parse_common_tags, but it deviates from convention. Either add _parse_common_tags to WebhookItem for consistency, or leave a short comment explaining why the simpler approach is safe.

status_change_reason is a mutable public attribute

webhook_item.py:61 — It's stored as a plain self.status_change_reason: str | None = None and can be set freely by callers. Since it's server-assigned and not serialized by update_req, a caller who sets it won't get an error but also won't get the intended behavior. Per the project's property pattern for server-assigned/read-only fields (id, created_at, etc.), this should be a getter-only property (no setter), or the docstring should explicitly say "server-set, do not write."

test_update doesn't assert the request body

test/test_webhook.py:184 — The test only validates response parsing. There's no m.last_request.text assertion confirming the correct XML was sent in the PUT body. The test_update_request_factory_* tests cover serialization independently, so coverage isn't missing, but an integration-level check tying the endpoint to its request body would be more complete.

Inconsistent -> None annotation sweep

test_webhook.py:91–120 — The PR adds -> None to exactly 4 of the existing test functions while leaving others without it. Either annotate all test functions in the file or none (the file had none before this PR).

Missing CHANGELOG entry

Per repo conventions, user-visible additions get a changelog bullet with the PR number. server.webhooks.update() is a new public method that qualifies.

Minor

webhook_item.py — update_req accesses webhook_item._event directly (intentional, since the property strips the prefix), but a brief inline comment would help readers understand why the private field is used instead of the public property.
webhooks_endpoint.py:131 — The docstring Returns description could match the phrasing used in other update methods for consistency.

Match the convention used by users_endpoint.update and datasources_endpoint.update so that fields set locally on a WebhookItem are preserved when the server's update response omits them. Previously the endpoint used WebhookItem.from_response(...)[0], which returned a fresh item populated only from server-supplied fields. - Adds WebhookItem._parse_common_tags matching the pattern in UserItem and WorkbookItem (name refers to XML common tags, not user-facing tags). - Adds test_update_preserves_locally_set_fields_omitted_by_server exercising the local-preservation semantics against a partial server response. Feedback from bcantoni on #1806.

jacalata · 2026-07-28T18:17:23Z

Valid. Landed the convention fix in 3f4b08a:

Added WebhookItem._parse_common_tags matching the pattern in UserItem and WorkbookItem (name refers to XML common tags, not Tableau item tags).
Reworked Webhooks.update() to copy.copy(item) + updated._parse_common_tags(...), matching users_endpoint.py:208 and datasources_endpoint.py:353.
New test test_update_preserves_locally_set_fields_omitted_by_server — sets local url, owner_id, is_enabled=False, mocks a partial server response omitting those three, asserts they survive.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds webhook update support and extends webhook parsing/serialization to include enablement state and status change reason.

Changes:

Introduces webhooks.update() endpoint (API v3.6) that preserves locally-set fields when server responses are partial.
Adds request factory support for webhook update requests.
Extends WebhookItem parsing/model fields for is_enabled and status_change_reason, plus new tests and XML asset.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
test/test_webhook.py	Adds tests for webhook update behavior, request serialization, and parsing `isEnabled`/`statusChangeReason`.
test/assets/webhook_update.xml	Adds fixture XML representing an updated webhook response.
tableauserverclient/server/request_factory.py	Adds `Webhook.update_req` to build update request bodies.
tableauserverclient/server/endpoint/webhooks_endpoint.py	Adds `WebhooksEndpoint.update` implementation with merge semantics.
tableauserverclient/models/webhook_item.py	Adds new fields and parsing/merge helper for webhook update responses.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+        if not webhook_item.id:
+            error = "Webhook item missing ID. Webhook must be retrieved from server first."
+            raise MissingRequiredFieldError(error)


+        if webhook_item._event is not None:
+            source = ET.SubElement(webhook, "webhook-source")
+            ET.SubElement(source, webhook_item._event)


+        webhook = ET.SubElement(xml_request, "webhook")
+        if webhook_item.name is not None:
+            webhook.attrib["name"] = webhook_item.name
+        if webhook_item.is_enabled is not None:
+            webhook.attrib["isEnabled"] = str(webhook_item.is_enabled).lower()


+def test_event_setter_full_source_name() -> None:
    """Full webhook-source-event- names should be accepted and stored as-is."""


Combined findings from the June 17 pre-merge code review and the recent Copilot pass: - **Missing test: isEnabled-only partial update** — a webhook update with only `is_enabled` set (no name, url, event) should produce an XML body that emits `isEnabled` but omits every other element/attr. Added `test_update_request_factory_partial_update_is_enabled_only`. - **Missing test: status_change_reason absent from response** — verify that when the server omits the `statusChangeReason` attribute entirely, `WebhookItem.status_change_reason` parses to None (not empty string, no crash). Added `test_status_change_reason_absent_from_response_is_none`. - **Missing test: update() on server < 3.6** — the `@api(version="3.6")` decorator should block, not proceed to the underlying PUT. Added `test_update_raises_on_server_below_3_6`. - **Missing test: new-style webhook-event-* through update_req** — round trip the newer event-name prefix through the update payload builder. Added `test_update_request_factory_new_style_event_name`. - **Copilot: docstring "stored as-is" is misleading** — the setter test for `webhook-source-event-*` names had a one-line docstring that implied the public `.event` getter also returns the full name. It doesn't: it strips the prefix for backward compat. Expanded the docstring to say so explicitly. - **Copilot: misleading error message on update() missing id** — the current text says "Webhook must be retrieved from server first", implying `webhooks.get()` is the only way. Callers can also set the id directly; broadened the message to mention both paths. No production-code behaviour change beyond the error-message text. 27 tests in test_webhook.py pass (23 existing + 4 new). Full suite: 879 passed, 1 skipped.

jacalata and others added 3 commits June 26, 2026 01:00

feat: add webhook update method and isEnabled/statusChangeReason fields

21b2cdd

Fixes #1135 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix: restore missing docstring on test_event_setter_none

9bd222f

The return-type annotation added in the branch accidentally dropped the docstring that was on test_event_setter_none. Restore it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

jacalata added 2 commits July 28, 2026 00:38

Merge origin/development into jac/webhook-update

ddbfbd5

jacalata changed the title ~~Add webhook update method with isEnabled/statusChangeReason~~ feat: Add webhook update method with isEnabled/statusChangeReason Aug 6, 2026

jacalata requested a lite review from Copilot August 6, 2026 21:19

jacalata enabled auto-merge (squash) August 6, 2026 21:19

Copilot AI reviewed Aug 6, 2026

View reviewed changes

Navigation Menu

Uh oh!

feat: Add webhook update method with isEnabled/statusChangeReason - #1806

feat: Add webhook update method with isEnabled/statusChangeReason#1806
jacalata wants to merge 6 commits into
developmentfrom
jac/webhook-update

jacalata commented Jun 26, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jun 26, 2026 •

edited

Loading

Uh oh!

bcantoni commented Jul 23, 2026

Uh oh!

jacalata commented Jul 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		def test_event_setter_full_source_name() -> None:
		"""Full webhook-source-event- names should be accepted and stored as-is."""

Uh oh!

Conversation

jacalata commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Schema compliance

Test plan

Uh oh!

github-actions Bot commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcantoni commented Jul 23, 2026

Code Review

Issues

Minor

Uh oh!

jacalata commented Jul 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jacalata commented Jun 26, 2026 •

edited

Loading

github-actions Bot commented Jun 26, 2026 •

edited

Loading